Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix npic false positive #1540

Open
wants to merge 5 commits into
base: register-npic
Choose a base branch
from

Conversation

augustin-v
Copy link
Contributor

@augustin-v augustin-v commented Feb 24, 2025

Hi @smoelius! I've researched on my own about how to filter out the false positives we are encountering(#1536) consisting of:

  • Complex URL schemas
  • Paths for demonstration, starting with $

I managed to make significant progress, especially with filtering out these two.
The first notable change from the lint is the switch to the fancy-regex crate, which allow more flexibility by using look-around assertions in regular expressions. Here's a detailed breakdown of the key improvements:

  1. URL filtering with fancy-regex
  • Added URL_REGEX pattern https?://\S+ to detect web URLs
  • Implemented URL range exclusion logic to avoid path checks in URLs
  1. Improved path pattern matching
  • Updated PATH_REGEX to support:
    • Dollar signs in paths (\$)
  • Added explicit exclusion for paths starting with:
    • URL protocols (http://, https://)
    • Web domains (www.)
    • Placeholder indicators ($)

These changes significantly reduce false positives while maintaining strong detection of actual broken path references. The fancy-regex crate enables complex pattern matching that would panic with the standard regex crate, particularly the negative lookbehind needed for accurate URL detection within comments.

The test suite has also been updated to verify these new cases, including:

  • URLs containing path-like fragments
  • Documentation comments with intentional examples
  • Placeholder paths using $DIR/path syntax

Looking forward to your feedback!

@augustin-v augustin-v requested a review from smoelius as a code owner February 24, 2025 12:22
@smoelius
Copy link
Collaborator

Sorry, I've just been busy with other things. I will review this within the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants